Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support alpha channel in as-hex #203

Merged

Conversation

ZehCnaS34
Copy link
Contributor

Related issue #182

Copy link
Collaborator

@WorldsEndless WorldsEndless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work with the tests

(-> (util/format "%2s" (util/int->string v 16))
(string/replace " " "0")))]
(str (apply str "#" (map hex-part [r g b]) )
(hex-part (int (* 256 a))))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert, but this looks like it might return the wrong thing when a=1. 🙇

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would make a good test case @ZehCnaS34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harold @WorldsEndless thanks! Instead of using an if expression to check if (= 1 a) I wanted to use 255 emplace of 256. But doing that would have affected the case where (= a 0.5)

Hopefully the if expression is alright.

@WorldsEndless
Copy link
Collaborator

I'll wait on the merge until one of the more experienced Gardeners chime in @noprompt @priyatam

Copy link
Collaborator

@WorldsEndless WorldsEndless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to test the alpha=1 cases per #203 (comment)

Copy link
Collaborator

@WorldsEndless WorldsEndless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! I'll give it a day before I do a merge on this, to wait for input.

(-> (util/format "%2s" (util/int->string v 16))
(string/replace " " "0")))]
(str (apply str "#" (map hex-part [r g b]) )
(if (= 1 a)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting approach; I'm always skeptical of using = to compare floating point numbers, but in this case I think it's okay due to how rgba works (specifically, how it ensures 0 <= a <= 1).

👍

In particular, (rgba->hex {:red 0 :green 0 :blue 0 :alpha 1.5}) produces a confusing answer, but perhaps that's okay.

Changing this line to (if (>= a 1) ... is one possible consideration.

Good work, by the way, love to see this library living/impoved.

@noprompt
Copy link
Owner

@WorldsEndless @harold I don't have the time to review these PRs, however, I trust both of you to make good decisions. 😄

@WorldsEndless WorldsEndless merged commit be9c73b into noprompt:master Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants